-
Notifications
You must be signed in to change notification settings - Fork 464
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
curve!: use constant-time compressed Ristretto equality testing #669
base: main
Are you sure you want to change the base?
curve!: use constant-time compressed Ristretto equality testing #669
Conversation
Note that dalek-cryptography/subtle#131 would also supply a marker trait that could be useful here to signal this "all equality is constant time" behavior. |
curve25519-dalek/src/ristretto.rs
Outdated
#[derive(Copy, Clone, Eq, PartialEq, Hash)] | ||
#[derive(Copy, Clone, Hash)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While it's unlikely to cause real-world breakages, I believe this is technically a breaking change.
Here's an example of what is possible with a derived PartialEq
which won't be possible with this change: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=33e85cff0772c968767a0417a8a7a541
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting! I was not aware of this behavior. Thoughts on the tradeoff between the functionality and the implications of the breaking change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would strongly agree with making this change if it weren't for the potential breakages
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ought this PR stay open until the next breaking release? I remain of the opinion that the benefit (consistency with safety goals) outweighs the cost.
e03cc24
to
326eeda
Compare
Yeah that's unfortunate. I really doubt anyone has ever used a |
326eeda
to
f36fd77
Compare
Given that this seems to be a desired change, I'll keep it open for the next breaking release. Feel free to close if this isn't the case! |
In line with the safety goals, this PR ensures that
CompressedRistretto
equality testing is always done in constant time.Previous work in #229 implemented
ConstantTimeEq
forCompressedRistretto
, but this is not used forEq
equality testing. It's already the case thatRistrettoPoint
andScalar
perform all equality testing in constant time; this PR unifies this behavior for compressed points as well.BREAKING CHANGE: As noted by @tarcieri, this can break certain uses of
match
.